-
Notifications
You must be signed in to change notification settings - Fork 1
(#154) Set Groups and Case Sensitivity to optional Parameters #155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
(#154) Set Groups and Case Sensitivity to optional Parameters #155
Conversation
|
Report: Report: clientLibrary - scala:2.12.17
|
…y-for-token-retrieval-in-client-lib-as-optional
dk1844
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is nice, but with a library we should meet additional standards (see comments below)
...brary/src/main/scala/za/co/absa/loginclient/tokenRetrieval/client/TokenRetrievalClient.scala
Show resolved
Hide resolved
| */ | ||
| def fetchRefreshToken(username: String, password: String): RefreshToken = { | ||
| fetchAccessAndRefreshToken(username, password, List.empty, false)._2 | ||
| def fetchRefreshToken(authMethod: AuthMethod): RefreshToken = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither the new method nor the existing (which should be present for some time as described above) seem to be covered with unit tests.
That is unfortunate, because the appropriate class TokenRetrievalClientTest already exists and it would be nice to have it covered. I see two options:
- You may need to mock the creation of
RestTemplate&KerberosRestTemplate(could be done by moving the creation to a method that you override in the test) to write the unit test properly. - A half-measure would be just to mock the
fetchTokenmethods in the class to check if those are receiving expected parameters. Not a complete unit test, but even this would be better than nothing if the option 1 cannot be reasonably done (I believe it should be).
...brary/src/main/scala/za/co/absa/loginclient/tokenRetrieval/client/TokenRetrievalClient.scala
Show resolved
Hide resolved
| class MockableTokenRetrievalClient( | ||
| host: String, | ||
| restTemplate: RestTemplate, | ||
| kerberosRestTemplate: KerberosRestTemplate) extends TokenRetrievalClient(host) { | ||
|
|
||
| override private[client] def fetchToken(issuerUri: String, username: String, password: String): String = { | ||
| val response: ResponseEntity[String] = restTemplate.exchange( | ||
| issuerUri, | ||
| HttpMethod.POST, | ||
| null, | ||
| classOf[String]) | ||
| response.getBody | ||
| } | ||
|
|
||
| override private[client] def fetchToken( | ||
| issuerUri: String, | ||
| keyTabLocation: Option[String], | ||
| userPrincipal: Option[String]): String = { | ||
| val response: ResponseEntity[String] = kerberosRestTemplate.exchange( | ||
| issuerUri, | ||
| HttpMethod.POST, | ||
| null, | ||
| classOf[String]) | ||
| response.getBody | ||
| } | ||
|
|
||
| override def refreshAccessToken( | ||
| accessToken: AccessToken, | ||
| refreshToken: RefreshToken): (AccessToken, RefreshToken) = { | ||
| val issuerUri = s"$host/token/refresh" | ||
| val jsonPayload: JsonObject = new JsonObject() | ||
| jsonPayload.addProperty("token", accessToken.token) | ||
| jsonPayload.addProperty("refresh", refreshToken.token) | ||
|
|
||
| val headers = new HttpHeaders() | ||
| headers.setContentType(MediaType.APPLICATION_JSON) | ||
| headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)) | ||
|
|
||
| val requestEntity = new HttpEntity[String](jsonPayload.toString, headers) | ||
|
|
||
| val response: ResponseEntity[String] = restTemplate.exchange( | ||
| issuerUri, | ||
| HttpMethod.POST, | ||
| requestEntity, | ||
| classOf[String] | ||
| ) | ||
| val jsonObject = JsonParser.parseString(response.getBody).getAsJsonObject | ||
| ( | ||
| AccessToken(jsonObject.get("token").getAsString), | ||
| RefreshToken(jsonObject.get("refresh").getAsString) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are already providing the mocked exchange templates, I assume you want the full solution. Good choice.
However, you do not need to override all the fetchToken and refreshAccessToken (also these should be rather tested, not overridden). I think all you need is this:
| class MockableTokenRetrievalClient( | |
| host: String, | |
| restTemplate: RestTemplate, | |
| kerberosRestTemplate: KerberosRestTemplate) extends TokenRetrievalClient(host) { | |
| override private[client] def fetchToken(issuerUri: String, username: String, password: String): String = { | |
| val response: ResponseEntity[String] = restTemplate.exchange( | |
| issuerUri, | |
| HttpMethod.POST, | |
| null, | |
| classOf[String]) | |
| response.getBody | |
| } | |
| override private[client] def fetchToken( | |
| issuerUri: String, | |
| keyTabLocation: Option[String], | |
| userPrincipal: Option[String]): String = { | |
| val response: ResponseEntity[String] = kerberosRestTemplate.exchange( | |
| issuerUri, | |
| HttpMethod.POST, | |
| null, | |
| classOf[String]) | |
| response.getBody | |
| } | |
| override def refreshAccessToken( | |
| accessToken: AccessToken, | |
| refreshToken: RefreshToken): (AccessToken, RefreshToken) = { | |
| val issuerUri = s"$host/token/refresh" | |
| val jsonPayload: JsonObject = new JsonObject() | |
| jsonPayload.addProperty("token", accessToken.token) | |
| jsonPayload.addProperty("refresh", refreshToken.token) | |
| val headers = new HttpHeaders() | |
| headers.setContentType(MediaType.APPLICATION_JSON) | |
| headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)) | |
| val requestEntity = new HttpEntity[String](jsonPayload.toString, headers) | |
| val response: ResponseEntity[String] = restTemplate.exchange( | |
| issuerUri, | |
| HttpMethod.POST, | |
| requestEntity, | |
| classOf[String] | |
| ) | |
| val jsonObject = JsonParser.parseString(response.getBody).getAsJsonObject | |
| ( | |
| AccessToken(jsonObject.get("token").getAsString), | |
| RefreshToken(jsonObject.get("refresh").getAsString) | |
| ) | |
| } | |
| class MockableTokenRetrievalClient( | |
| host: String, | |
| restTemplate: RestTemplate, | |
| kerberosRestTemplate: KerberosRestTemplate) extends TokenRetrievalClient(host) { | |
| override def createRestTemplate(): RestTemplate = restTemplate | |
| override def createKerberosRestTemplate( /* opt params or two methods? */ ): KerberosRestTemplate = kerberosRestTempl |
while updating the with
case class TokenRetrievalClient(host: String) {
private[client] def createKerberosRestTemplate( /* opt params or two methods? */ ): KerberosRestTemplate = {
new KerberosRestTemplate( /* opt params?*/)
}
private[client] def createRestTemplate(): RestTemplate = {
new RestTemplate()
}
// and wherever the `new RestTemplate()` or `new KerberosRestTemplate(...)` is used within `TokenRetrievalClient`, you replace it with the methods above.This way, you only override the very minimal to get your exchange mocks in -- while the rest of the TokenRetrievalClient stays original as is correctly subjected to unit testing.
Release Notes:
Closes: #154